-
Notifications
You must be signed in to change notification settings - Fork 274
Feat/balancer swap #2290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devnet-ready
Are you sure you want to change the base?
Feat/balancer swap #2290
Conversation
…rate_reset_unactive_sn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not operating square root of prices anymore, so renamed this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file contains the heart of balancer math. All swaps come through this logic. Please read the comments to understand what we're doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is where uniswap v3 logic used to be. Reworked to use balancer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we can remove all swap v3 maps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed swap v3 maps and replaced with balancer maps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so much nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unline uniswap v3, balancer swap is always a single step, but yet, swap_step provides cleaner separation of swap logic, and I decided to preserve the structure. See how much simpler it is now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adjust_protocol_liquidity returns the actual adjustment that needs to be made to reserves, which includes previously collected tao and alpha fees.
When tao fees are charged, the tao is coming from user balance to FeesTao map, not accounted for in SubnetTAO, so it needs to be added here.
When alpha fees are charged, the alpha is coming from user's stake, and is also not accounted for in SubnetAlphaIn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still an approximation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No formula for this exists, but we could do newtons method. We have an implementation if you want it.
@gztensor see discord.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continue removing storage for provided reserves here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Positions are unused atm, but I preserved the struct for later use, when we have user liquidity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of uniswap v3 logic. Not needed anymore.
0xcacti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will provide one more review, looking at pallets/swap/src/pallet/swap_step.rs and also pallets/swap/src/pallet/impls.rs
it got late and we don't want to lose our progress.
| Perquintill::from_rational(1u128, 2u128) | ||
| }; | ||
|
|
||
| self.set_quote_weight(new_reserve_weight) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like you forgot to set the base weight here? we only set the quote weight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how could we have possibly gotten through testing? are we missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind, base is always looked up as 1 - quote
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just delete this whole function in which we have changes. The function isn't used anywhere other than in a test, and the logic for emissions is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love cleaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point. correc.t
| Self::add_liquidity_at_index(netuid, position.tick_low, liquidity_delta, false); | ||
| Self::add_liquidity_at_index(netuid, position.tick_high, liquidity_delta, true); | ||
| } | ||
| ) -> (TaoCurrency, AlphaCurrency) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is high level correct.
of course, you know that I think it's really weird the way that the reserves are in another pallet so we update them in the calling context rather than here, but I suppose my view there doesn't really matter.
As is, it's correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't entirely understand this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so much nicer.
| /// Constraints limit balancer weights within certain range of values: | ||
| /// - Both weights are above minimum | ||
| /// - Sum of weights is equal to 1.0 | ||
| fn check_constraints(quote: Perquintill) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are working on inventing a more advanced theory here for when we reach the MIN_WEIGHT what we should do. All good for now, but these constraints may change in the future.
| pub fn update_weights_for_added_liquidity( | ||
| &mut self, | ||
| tao_reserve: u64, | ||
| alpha_reserve: u64, | ||
| tao_delta: u64, | ||
| alpha_delta: u64, | ||
| ) -> Result<(), BalancerError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to make sure that here we are passing in the actual_delta,
I.e. the quantity injected + fees
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked and it is because it's only called by adjust_protocol_liqudity which handles this.
| /// y - tao reserve | ||
| /// w1 - base weight | ||
| /// w2 - quote weight | ||
| pub fn calculate_current_liquidity(&self, tao_reserve: u64, alpha_reserve: u64) -> u64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need this function for anything? could probably get rid of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if we want user liquidity at some point in the future.
Description
Implement PalSwap (similar to balancer swap) to replace Uniswap V3.
Type of Change
Breaking Change
This PR changes the following externally accessible entities:
add_liqudiityextrinsic: Remove tick_low and tick_high parameters.Positionsmap is replaced withPositionsV2mapChecklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctly